Async BumpTransactionEventHandler #3752

joostjager · 2025-04-28T19:09:23Z

Converts BumpTransactionEventHandler to async.

Changes the CoinSelectionSource and WalletSource traits to be async and provides WalletSourceSyncWrapper as a helper for users that want to implement a sync wallet source.

TestWalletSource is kept sync, to prevent a cascade of async conversions in tests.

Fixes #3540

ldk-reviews-bot · 2025-04-28T19:09:26Z

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

codecov · 2025-05-01T11:16:59Z

Codecov Report

Attention: Patch coverage is 94.75410% with 16 lines in your changes missing coverage. Please review.

Project coverage is 89.78%. Comparing base (98297a6) to head (96b704f).

Files with missing lines	Patch %	Lines
lightning/src/events/bump_transaction.rs	91.97%	4 Missing and 9 partials ⚠️
lightning/src/ln/functional_tests.rs	96.29%	1 Missing ⚠️
lightning/src/ln/monitor_tests.rs	98.94%	1 Missing ⚠️
lightning/src/util/test_utils.rs	83.33%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##             main    #3752    +/-   ##
========================================
  Coverage   89.78%   89.78%            
========================================
  Files         159      159            
  Lines      128672   128778   +106     
  Branches   128672   128778   +106     
========================================
+ Hits       115522   115623   +101     
- Misses      10476    10478     +2     
- Partials     2674     2677     +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

tnull

Did a first / quickish pass. Generally looks good so far.

tnull · 2025-05-15T13:17:48Z

lightning/Cargo.toml

@@ -48,12 +48,14 @@ backtrace = { version = "0.3", optional = true }

 libm = { version = "0.2", default-features = false }
 inventory = { version = "0.3", optional = true  }
+tokio = { version = "1.35", features = [ "macros", "rt" ], optional = true }


Please set default-features = false here and below.

Added. Why is this? I looked at the tokio crate and it seems there are no default features?

It's 'best practice' and will also make sure we'd not add arbitrary dependencies if tokio decided to add default features in some release.

tnull · 2025-05-15T13:21:04Z

lightning/src/events/bump_transaction.rs

-					*commitment_tx_fee_satoshis,
-					anchor_descriptor,
-				) {
+				if let Err(_) = self


IMO these would be a bit more readable if we did the the whole call including .await on a separate line and then simply did if res.is_err() { .. }

Tried it, but I don't think it is an improvement with that temp var.

let res = self .handle_channel_close( *claim_id, *package_target_feerate_sat_per_1000_weight, commitment_tx, *commitment_tx_fee_satoshis, anchor_descriptor, ) .await; if res.is_err() { log_error!( self.logger, "Failed bumping commitment transaction fee for {}", commitment_tx.compute_txid() ); }

Also tried with map_err, but that doesn't work all that well with the parent fn without a return result.

Also tried with map_err, but that doesn't work all that well with the parent fn without a return result.

How about:

self.handle_channel_close( *claim_id, *package_target_feerate_sat_per_1000_weight, commitment_tx, *commitment_tx_fee_satoshis, anchor_descriptor, ) .await .unwrap_or_else(|_| { log_error!( self.logger, "Failed bumping commitment transaction fee for {}", commitment_tx.compute_txid() ); });

Ah this is nicer. Wasn't aware of the unwrap_or_else on result. Updated.

lightning/src/events/bump_transaction.rs

tnull · 2025-05-15T13:27:08Z

lightning/src/ln/functional_tests.rs

+	#[allow(clippy::expect_used, clippy::diverging_sub_expression)]
+	{
+		return tokio::runtime::Builder::new_current_thread()
+			.enable_all()


FWIW, I don't think we need to enable_all here, could just actually enable features we need. Same goes for all the tests that use [tokio::test], although not sure if it makes a big difference in practice.

I just expanded the macro. Can make customizations, but maybe it is better to keep to the original expansion? Also if it doesn't matter in practice...

Yeah, no strong opinion. If we increase the number of tests using tokio, we might want to some benchmarks though, as the runtime of cargo test accumulates over time, so every second we can squeeze out of it would be appreciated.

Removed then. If we get more of these xtests, we might need to duplicate the macro or something like that.

lightning/src/events/bump_transaction.rs

tnull · 2025-05-15T13:52:19Z

lightning/src/events/bump_transaction.rs

+			let configs = [(false, false), (false, true), (true, false), (true, true)];
+			let mut last_err = None;
+			for (force_conflicting_utxo_spend, tolerate_high_network_feerates) in configs {
+				log_debug!(
 				self.logger,
 				"Attempting coin selection targeting {} sat/kW (force_conflicting_utxo_spend = {}, tolerate_high_network_feerates = {})",
 				target_feerate_sat_per_1000_weight,
 				force_conflicting_utxo_spend,
 				tolerate_high_network_feerates
 			);


nit: Indent off here.

I don't see the indent off. The closing parenthesis?

Actually the parenthesis and all arguments of log_debug too (they need to be indented by one tab).

I don't see the problem anymore in the diff now, but github still shows it on the old code fragment? In my IDE it all looks good with tabs, so I think it is solved.

Nope, still there on HEAD:

Ah, happened in the next commit. Fixed now.

ldk-reviews-bot · 2025-05-15T13:52:43Z

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

Async closures are complicated. Preparatory commit.

joostjager · 2025-05-16T08:24:52Z

@tnull latest changes here

Will now push the rebase

tnull

Did another pass. I have yet to verify it doesn't break anything in LDK Node, but otherwise LGTM (mod the pending comments here).

lightning/src/events/bump_transaction.rs

tnull · 2025-05-16T08:34:05Z

lightning/src/events/bump_transaction.rs

 					log_error!(
 						self.logger,
-						"Failed bumping commitment transaction fee for {}",
+						"Failed bumping channel close transaction for {}",


Hmm, why was the previous message wrong?

Ai ai ai I am so embarrassed here. This and the other ones are LLM completions 😞

lightning/src/events/bump_transaction.rs

lightning/src/ln/functional_tests.rs

tnull · 2025-05-16T08:52:32Z

lightning/src/events/bump_transaction.rs

+			let configs = [(false, false), (false, true), (true, false), (true, true)];
+			let mut last_err = None;
+			for (force_conflicting_utxo_spend, tolerate_high_network_feerates) in configs {
+				log_debug!(
 				self.logger,
 				"Attempting coin selection targeting {} sat/kW (force_conflicting_utxo_spend = {}, tolerate_high_network_feerates = {})",
 				target_feerate_sat_per_1000_weight,
 				force_conflicting_utxo_spend,
 				tolerate_high_network_feerates
 			);


Nope, still there on HEAD:

Convert methods that need to be async when the wallet traits become async.

lightning/src/events/bump_transaction.rs

tnull

Looks good from my side, mod one (likely) LLM-induced change that you overlooked.

lightning/src/events/bump_transaction.rs

It seems that the compiler doesn't recognize the drop and complains that the mutex crosses an await (introduced in later commit), even though it doesn't.

Async closures are complicated. Preparatory commit.

Changes the CoinSelectionSource and WalletSource traits to be async and provides WalletSourceSyncWrapper to as a helper for users that want to implement a sync wallet source. TestWalletSource is kept sync, to prevent a cascade of async conversions in tests.

tnull

LGTM, mod second reviewer feedback

joostjager force-pushed the async-wallet-bump branch 3 times, most recently from d9b1c75 to 873516b Compare May 1, 2025 11:08

joostjager force-pushed the async-wallet-bump branch 6 times, most recently from 4b4b898 to 3816c66 Compare May 5, 2025 11:37

joostjager force-pushed the async-wallet-bump branch 13 times, most recently from 1a86521 to e1509f1 Compare May 14, 2025 09:55

joostjager changed the title ~~Async wallet bump~~ Async BumpTransactionEventHandler May 14, 2025

joostjager marked this pull request as ready for review May 14, 2025 11:06

joostjager requested a review from tnull May 14, 2025 11:06

joostjager added the weekly goal Someone wants to land this this week label May 14, 2025

tnull reviewed May 15, 2025

View reviewed changes

joostjager force-pushed the async-wallet-bump branch from e1509f1 to 991d505 Compare May 15, 2025 14:56

joostjager requested a review from tnull May 15, 2025 14:57

joostjager force-pushed the async-wallet-bump branch from 991d505 to 7f67bb0 Compare May 16, 2025 08:20

Rewrite closure check_htlc_retry

5c9ef40

Async closures are complicated. Preparatory commit.

joostjager force-pushed the async-wallet-bump branch from 7f67bb0 to 6fd5e5b Compare May 16, 2025 08:25

joostjager requested a review from TheBlueMatt May 16, 2025 08:25

tnull reviewed May 16, 2025

View reviewed changes

Prepare BumpTransactionEventHandler for async conversion

62000fe

Convert methods that need to be async when the wallet traits become async.

joostjager force-pushed the async-wallet-bump branch from 6fd5e5b to 96b704f Compare May 16, 2025 10:06

joostjager requested a review from tnull May 16, 2025 10:08

joostjager commented May 16, 2025

View reviewed changes

lightning/src/events/bump_transaction.rs Show resolved Hide resolved

tnull reviewed May 16, 2025

View reviewed changes

lightning/src/events/bump_transaction.rs Show resolved Hide resolved

joostjager added 3 commits May 16, 2025 13:53

Add scope instead of drop

02bca53

It seems that the compiler doesn't recognize the drop and complains that the mutex crosses an await (introduced in later commit), even though it doesn't.

Rewrite closure

0486af6

Async closures are complicated. Preparatory commit.

joostjager force-pushed the async-wallet-bump branch from 96b704f to 91bc001 Compare May 16, 2025 11:53

joostjager requested a review from tnull May 16, 2025 11:56

tnull reviewed May 16, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Async BumpTransactionEventHandler #3752

Async BumpTransactionEventHandler #3752

joostjager commented Apr 28, 2025 •

edited

Loading

ldk-reviews-bot commented Apr 28, 2025 •

edited

Loading

codecov bot commented May 1, 2025 •

edited

Loading

tnull left a comment •

edited

Loading

tnull May 15, 2025

joostjager May 15, 2025

tnull May 16, 2025

tnull May 15, 2025

joostjager May 15, 2025 •

edited

Loading

joostjager May 15, 2025

tnull May 16, 2025

joostjager May 16, 2025

tnull May 15, 2025

joostjager May 15, 2025 •

edited

Loading

tnull May 16, 2025

joostjager May 16, 2025

tnull May 15, 2025

joostjager May 15, 2025

tnull May 16, 2025

joostjager May 16, 2025

tnull May 16, 2025

joostjager May 16, 2025 •

edited

Loading

ldk-reviews-bot commented May 15, 2025

joostjager commented May 16, 2025

tnull left a comment

tnull May 16, 2025

joostjager May 16, 2025

tnull May 16, 2025

tnull left a comment

tnull left a comment

Async BumpTransactionEventHandler #3752

Are you sure you want to change the base?

Async BumpTransactionEventHandler #3752

Conversation

joostjager commented Apr 28, 2025 • edited Loading

ldk-reviews-bot commented Apr 28, 2025 • edited Loading

codecov bot commented May 1, 2025 • edited Loading

Codecov Report

tnull left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joostjager May 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joostjager May 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joostjager May 16, 2025 • edited Loading

Choose a reason for hiding this comment

ldk-reviews-bot commented May 15, 2025

joostjager commented May 16, 2025

tnull left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull left a comment

Choose a reason for hiding this comment

tnull left a comment

Choose a reason for hiding this comment

joostjager commented Apr 28, 2025 •

edited

Loading

ldk-reviews-bot commented Apr 28, 2025 •

edited

Loading

codecov bot commented May 1, 2025 •

edited

Loading

tnull left a comment •

edited

Loading

joostjager May 15, 2025 •

edited

Loading

joostjager May 15, 2025 •

edited

Loading

joostjager May 16, 2025 •

edited

Loading